Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow exporting terminal buffer into file via tab context menu #11062

Merged
4 commits merged into from
Aug 31, 2021

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Aug 27, 2021

Summary of the Pull Request

Naive implementation of exporting the text buffer of the current pane
into a text file triggered from the tab context menu.

Disclaimer: this is not an export of the command history,
but rather just a text buffer dumped into a file when asked explicitly.

References

Should provide partial solution for #642.

Detailed Description of the Pull Request / Additional comments

The logic is following:

  • Open a file save picker
    • The location is Downloads folder (should be always accessible)
    • The suggest name of the file equals to the pane's title
    • The allowed file formats list contains .txt only
  • If no file selected stop
  • Lock terminal
  • Read all lines till the cursor
  • Format each line by removing trailing white-spaces and adding CRLF if not wrapped
  • Asynchronously write to selected file
  • Show confirmation

As the action is relatively fast didn't add a progress bar or any other UX.
As the buffer is relatively small, holding it entirely in the memory rather than
writing line by line to disk.

Closes #642

@Don-Vito
Copy link
Contributor Author

Don-Vito commented Aug 27, 2021

@zadjii-msft, @DHowett

  • The implementation is quite naive and limited:
    • It contains only the lines of the buffer
    • It is not history: there are no timestamps or lines that were altered.
    • It is not ongoing logging, you actually need to export the file manually
  • I am guessing there might be less people happy with this feature than the ones annoyed with its limitations
  • Yet probably this could be a good place to start with
  • Unrelated to the above
    • I was not sure what is the most efficient way to export the buffer, so I took the simplest approach. Please let me know how it can be done better.
    • I didn't bother to write file line by line as the buffer is small. Please let me know if you think this should be changes at this stage.

@Don-Vito
Copy link
Contributor Author

ExportPane

The icon is:
image

@Don-Vito
Copy link
Contributor Author

Actually I don't like it.. Probably need to move the export into Control itself

@Don-Vito Don-Vito marked this pull request as draft August 27, 2021 07:29
@Don-Vito Don-Vito marked this pull request as ready for review August 27, 2021 08:59
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So excited for this feature to land! Thank you!

I was debating mentioning TextBuffer::GetText() but that seems too overly complicated than just iterating through the buffer and removing the whitespace directly. Not worth it.

src/cascadia/TerminalControl/ControlCore.h Outdated Show resolved Hide resolved
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I think needs more discussion before merging. I know you called this out in the related issue/in a comment on this PR, but yeah --- TermControl right now has no true external hookups except for throwing text into a connection and getting text from that connection back on screen. Any communication with the outside world is mediated through its event handlers ("I want to paste!" "Copy this text to the clipboard for me") and interfaces (ITerminalConnection's read and write). In the long-term future, it would be ideal if we could make it even less coupled to Terminal-the-app; imagine somebody like Visual Studio taking a dependency on the UWP version of the control instead of WPF, and it having a file-writing-sized hole in it.

ControlCore will eventually run on Windows 7, because ~ ~ hand-wavey annoying things regarding the WPF control and how i want to unify all the interaction models ~ ~, as will ControlInteractivity ... which makes taking a hard dependency on the pretty bad, if i am honest W.S.Storage APIs harder for me to digest. I dunno.

Thoughts?

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 27, 2021
@Don-Vito
Copy link
Contributor Author

@DHowett - I see your point. Before my last commit the logic was in the App. I am reverting it.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 27, 2021
@zadjii-msft
Copy link
Member

  • The implementation is quite naive and limited:

    • It contains only the lines of the buffer
    • It is not history: there are no timestamps or lines that were altered.
    • It is not ongoing logging, you actually need to export the file manually
  • I am guessing there might be less people happy with this feature than the ones annoyed with its limitations

I think that's absolutely fine. I think that incremental progress is more important than wholly complete in the first go. IMO this is a good place to start, while we think on how we want to actually express settings for the various options people all want. Auto-logging to files with formatted names, timestamps - that all basically needs a whole spec dedicated to it, but this can be done without all that planning.

@Don-Vito If you're interested in doing more auto-logging stuff, I can try and throw a spec together. Something more lightweight like #9365, so we can get you unblocked and a list of things to do. If you want to work on other stuff though, that's absolutely fine with me ☺️

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise, yea this is perfect for what it should do. But now you've got me thinking of all the other related things that this should do and I'm excited for those. I might end up writing the spec anyways 😝

if (const auto control{ tab.GetActiveTerminalControl() })
{
const FileSavePicker savePicker;
savePicker.as<IInitializeWithWindow>()->Initialize(*_hostingHwnd);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so just to be sure, this will work when the Terminal is elevated (run as admin), right? I want to make sure we don't run into the same issue that forced #9760

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do believe that it will crash.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is pickers, of all sorts, through the W.S API that is broken, not any specific individual use. We may try/catch this one, but... the best we can do is simply not pop up a dialog when Admin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is pickers, of all sorts, through the W.S API that is broken, not any specific individual use. We may try/catch this one, but... the best we can do is simply not pop up a dialog when Admin.

Frick. I'll stick that on #9700

savePicker.as<IInitializeWithWindow>()->Initialize(*_hostingHwnd);
savePicker.SuggestedStartLocation(PickerLocationId::Downloads);
const auto fileChoices = single_threaded_vector<hstring>({ L".txt" });
savePicker.FileTypeChoices().Insert(RS_(L"PlainText"), fileChoices);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait should "Plain Text" be localizable? I guess it should be, huh.


if (page && tab)
{
page->_ExportTab(*tab);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an aside: we may want to (in a future PR) turn this into an action, that could let the user export the pane to a file from the command palette too, not just the context menu. But now I'm thinking of all the other kinds of actions we could have for this feature...

@Don-Vito
Copy link
Contributor Author

Code-wise, yea this is perfect for what it should do. But now you've got me thinking of all the other related things that this should do and I'm excited for those. I might end up writing the spec anyways 😝

Cool! We absolutely need a spec to write! I would absolutely love to work on this feature (I need it as well). My major challenge is as usual: the time - I hope I will get to it (unlike the broadcast to multiple panes).

@zadjii-msft zadjii-msft added Needs-Discussion Something that requires a team discussion before we can proceed and removed Needs-Discussion Something that requires a team discussion before we can proceed labels Aug 30, 2021
@DHowett DHowett dismissed their stale review August 31, 2021 01:39

DIsmissing my own review in case folks want to move ahead like this :)

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 31, 2021
@ghost
Copy link

ghost commented Aug 31, 2021

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit c089ae0 into microsoft:main Aug 31, 2021
@@ -1211,13 +1211,31 @@ namespace winrt::TerminalApp::implementation
splitTabMenuItem.Icon(splitTabSymbol);
}

Controls::MenuFlyoutItem exportTabMenuItem;
{
// "Split Tab"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: says "split" 😁

@ghost ghost added the Area-UserInterface Issues pertaining to the user interface of the Console or Terminal label Sep 7, 2021
@ghost ghost added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Sep 7, 2021
ghost pushed a commit that referenced this pull request Oct 1, 2021
Just like in #9760, we can't actually use the UWP file picker API, because it will absolutely not work at all when the Terminal is running elevated. That would prevent the picker from appearing at all. So instead, we'll just use the shell32 one manually. 

This also gets rid of the confirmation dialog, since the team felt we didn't really need that. We could maybe replace it with a Toast (#8592), but _meh_

* [x] closes #11356
* [x] closes #11358
* This is a lot like #9760
* introduced in #11062
* megathread: #9700
@ghost
Copy link

ghost commented Oct 20, 2021

🎉Windows Terminal Preview v1.12.2922.0 has been released which incorporates this pull request.:tada:

Handy links:

@zadjii-msft zadjii-msft mentioned this pull request Jan 5, 2022
3 tasks
ghost pushed a commit that referenced this pull request Jan 12, 2022
This adds an action for the context menu entry we added in #11062. That PR added support for exporting the buffer, exclusively through the tab item's context menu. This adds an action that can additionally be bound, which also can export the buffer to a file. This action accepts a `path` param. If empty/ommitted, then the Terminal will prompt for the file to export the buffer to. 

* Does a part of #9700
* Spec in #11090, but I doubt this is contentious
* [x] This will satisfy #12052
* [x] I work here
* [x] docs added: MicrosoftDocs/terminal#479
ghost pushed a commit that referenced this pull request Dec 13, 2022
### ⇒ [doc link](https://github.com/microsoft/terminal/blob/dev/migrie%2Fs%2F642-logging/doc/specs/drafts/%23642%20-%20Buffer%20Exporting%20and%20Logging/%23642%20-%20Buffer%20Exporting%20and%20Logging.md) ⇐

## Summary of the Pull Request

This is an intentionally brief spec to address the full scope of #642. The
intention of this spec is to quickly build consensus around all the features we
want for logging, and prepare an implementation plan.

### Abstract

> A common user need is the ability to export the history of a terminal session to
> a file, for later inspection or validation. This is something that could be
> triggered manually. Many terminal emulators provide the ability to automatically
> log the output of a session to a file, so the history is always captured. This
> spec will address improvements to the Windows Terminal to enable these kinds of
> exporting and logging scenarios.

## PR Checklist
* [x] Specs: #642
* [x] References: #5000, #9287, #11045, #11062
* [x] I work here

## Detailed Description of the Pull Request / Additional comments
_\*<sup>\*</sup><sub>\*</sub> read the spec  <sub>\*</sub><sup>\*</sup>\*_

## Open Discussion
* [ ] What formatting string syntax and variables do we want to use?
* [ ] How exactly do we want to handle "log printable output"? Do we include backspaces? Do we only log on newlines?
* [ ] > maybe consider even simpler options like just `${date}` and `${time}`, and allow for future variants with something like `${date:yyyy-mm-dd}` or `${time:hhmm}`
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: export console history as txt
4 participants